feat(channels): bound session lifetime with sessionRotation - #8927
feat(channels): bound session lifetime with sessionRotation#8927qwen-code-dev-bot wants to merge 21 commits into
Conversation
A channel route reuses its session forever, so a long-lived route grows until it passes the model's context window — after which every message on that route fails while the rest of the channel keeps working. Add a per-channel `sessionRotation` option with `maxTurns` and `maxAgeHours` bounds. When a route's session is past a bound, the next message starts a fresh session on it. Counters persist alongside the routes so a daemon restart cannot reset them, and channels without a bound configured skip the bookkeeping entirely, keeping their on-disk route shape and per-message write behavior unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
the
review/self-reported
|
Thanks for the quick iteration! Template looks good ✓ Problem: observed, not theoretical. Linked issue #8926 documents the incident — a Direction: aligned. Chat-channel routes have no natural end, an opt-in per-channel bound is the right knob, and a token-based bound stays out of scope honestly (it would need a bridge capability that doesn't exist today). Size: cross-package ( Approach: the previous blocker got fixed the better of the two ways: instead of adding caller-side wiring, rotation registration moved into the Risk: no elevated risk signals — none of the changed files match the revert-correlated paths. Moving on to code review. 🔍 中文说明感谢快速迭代！ 模板完整 ✓ 问题：真实观测，不是理论假设。关联 issue #8926 记录了事故—— 方向：对齐。聊天频道路由没有自然终点，按频道可选配置限度是正确的开关；基于 token 的限度诚实地留在范围外（需要当前不存在的 bridge 能力）。 规模：跨包改动（ 方案：上一个阻塞项用了更好的方式修复：注册不是加在调用方，而是移进了 风险：无升级风险信号——改动文件均不命中与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRe-review at the new head. Before reading the diff I'd have fixed last run's blocker with one line of caller-side wiring; the PR did the structurally better thing instead — rotation registration now lives in the The machinery added since the last review reads clean under tracing:
Non-blocking notes:
TestingUnattended CI run — per policy I don't build or execute PR code; the evidence below is the PR's own CI on the reviewed commit, fetched via API, plus the maintainer's real-stack verification report. No TUI surface in this PR, so no tmux lane.
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。
Beyond the suite, @wenshao ran a two-sided real-stack verification at exactly this commit (his report above): a real extension-loaded channel against a recording model server plus an ACP wire tap, standalone and daemon legs — all twelve behavioral claims held, including the ones this review traces statically (rotation at the bound, per-route isolation, restart-persisted counters, in-place upgrade of pre-rotation stores, deferral under a running turn, discard of the retired session). That also closes last run's open gap: the standalone leg drove 中文说明代码审查（按新 head 复审）：读 diff 之前，我本来打算用一行调用方接线修复上次的阻塞项；PR 选了结构上更好的做法——轮换注册移入 上次审查之后新增的机制经追踪是干净的：
非阻塞提醒：
测试：无人值守 CI 运行——按策略不构建、不执行 PR 代码；以上证据来自 PR 自身在受审 commit 上的 CI（经 API 获取）及维护者的真实链路验证报告。本 PR 无 TUI 面，因此没有 tmux 环节。 套件之外，@wenshao 恰在此 commit 上做了双向真实链路验证（见其报告）：扩展加载的真实频道 + 记录型模型服务 + ACP 线协议探针，覆盖独立腿与守护进程腿——十二条行为声明全部成立，包括本次静态审查追踪到的各项（到限轮换、按路由隔离、计数跨重启持久化、旧存储原地升级、回合中推迟、退役会话回收）。这也关闭了上次运行的缺口：独立腿实际驱动了 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — approve. The blocker from the last round is fixed the structurally better way, the new retirement machinery traces clean, CI is green, and a maintainer verified the behavior end to end on this exact commit. Stepping back: my independent sketch of this fix was caller-side wiring; the PR's answer — move registration into What keeps this at 4 rather than 5 is the residue, all non-blocking: the restart-defers-age-only-bound gap @wenshao measured (his suggested one-line On verification: the suite pins the feature at both layers (router semantics and channel wiring), and the maintainer's two-sided real-stack run on this commit — recording model server, ACP tap, standalone and daemon legs — held all twelve behavioral claims, including the standalone launch mode that sank the previous revision. CI is fully settled on this commit (no pending runs), and @wenshao's approval already stands on it; the approval below is pinned to the reviewed commit and supersedes my earlier change request. 中文说明置信度：4/5 —— 批准。上一轮的阻塞项以结构上更好的方式修复，新的退役机制经追踪无问题，CI 全绿，且维护者已在此 commit 上端到端验证了行为。 退一步看：我对这个修复的独立设想是调用方接线；PR 的答案——把注册移进 停在 4 而不是 5 的原因是遗留项，均不阻塞：@wenshao 实测出的"重启可推迟纯年龄限度"缺口（他建议的一行 验证方面：套件在路由器语义与频道接线两个层面钉住了功能；维护者在此 commit 上的双向真实链路运行——记录型模型服务、ACP 探针、独立腿与守护进程腿——十二条行为声明全部成立，包括曾让上一版折戟的独立启动模式。CI 在此 commit 上已完全收敛（无 pending 运行），@wenshao 的批准已在该 commit 上；下方的批准锚定在受审 commit，并取代我此前的修改请求。 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
@qwen-code-dev-bot One fix needed before this can land: sessionRotation is never registered in single-channel mode — startSingle in packages/cli/src/commands/channel/start.ts passes a router it never calls setChannelRotation on, so the bound silently does not apply there (and the ChannelBase self-registration skips that path because a router is present). One line plus a test; full details in my review comment above. 🙏
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
|
🤖 Addressed the latest review feedback (round 1/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/10 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #8927Feedback points and dispositions1. [CHANGES_REQUESTED] Confirmed:
2. Failed check: Reproduced the unit suite locally (clean
The full verification stack ( Conflict notesNone — VerificationCommands actually run on the updated tree (clean CI-equivalent environment: fresh
中文说明Autofix 审查轮次 — PR #8927反馈点及处理1. [CHANGES_REQUESTED] 单频道模式下 已确认：
2. 失败检查： 在本地复现了单测套件（先干净
完整校验栈（ 冲突说明无 — Verification（验证记录）在更新后的代码树上实际执行的命令（干净的 CI 等价环境：全新
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 10 this bot stops and leaves the PR for a human. · 有空请复审；第 10 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
🤖 AutoFix updated a stale base — the fix did not pass verification, but this PR was behind What I found before stopping: AutoFix blocked: cannot attribute red
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): This PR adds a per-channel sessionRotation config optio...: none — all checks I started completed within budget.; This PR adds a per-channel sessionRotation config optio...: none — all checks I started were completed. Note: I did not fetch the PR's existing comment thread (not in scope of my reads); if an unresolved Critical exists …; This PR adds a per-channel sessionRotation config optio...: none — all checks I started completed within budget..
Test Plan (not a blocker): 158 tests passed — this review observed 1023, 290, 19069, 297, 266, 205, 59, 17, 134, 71 passed.
中文说明
未探索到全部深度（达到工具调用预算）：This PR adds a per-channel sessionRotation config optio...：none — all checks I started completed within budget.；This PR adds a per-channel sessionRotation config optio...：none — all checks I started were completed. Note: I did not fetch the PR's existing comment thread (not in scope of my reads); if an unresolved Critical exists …；This PR adds a per-channel sessionRotation config optio...：none — all checks I started completed within budget.。
Test Plan（非阻断）：158 tests passed — this review observed 1023, 290, 19069, 297, 266, 205, 59, 17, 134, 71 passed。
— qwen3.8-max via Qwen Code /review (v0.21.9)
|
🤖 Addressed the latest review feedback (round 3/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/10 轮）。改动内容与我反驳保留之处如下： PR #8927 review feedback — round summaryAll 20 findings (2 Critical, 18 Suggestion) from the automated review were Critical findingsR1-1 — Rotation leaked every retired session (SessionRouter.ts)Fixed.
R1-2 — Rotation fired while the outgoing session had an active turn (SessionRouter.ts)Fixed. Rotation now defers while the outgoing session still has a turn
Suggestions
Conflict
VerificationCommands actually run (after the fix commit):
中文说明PR #8927 评审反馈 — 本轮总结自动评审的 20 条发现（2 条严重、18 条建议）均已对照代码核实，并在一个提交 严重发现R1-1 — 轮换泄漏了每一个被退役的会话（SessionRouter.ts）已修复。
R1-2 — 轮换在旧会话仍有活动回合时触发（SessionRouter.ts）已修复。 当旧会话仍有回合在运行或排队时，轮换推迟到下一条消息落实限度
建议
冲突
验证实际执行的命令（修复提交之后）：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 10 this bot stops and leaves the PR for a human. · 有空请复审；第 10 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review triage — no action required (PR #8927)Triaged all feedback newer than the last evaluation (2026-08-11T16:21:48Z). This round contains nothing actionable:
No changes were made; the branch stays at its current head commit. 中文说明Autofix 评审分诊 — 无需处理（PR #8927）已对上次评估（2026-08-11T16:21:48Z）之后新增的全部反馈进行分诊。本轮没有任何需要处理的内容：
未做任何修改；分支保持在当前 head 提交。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
Verification report — real-stack run of
|
| commit | |
|---|---|
| PR head | 6dbca59 (fix(channels): retire rotated sessions safely and harden rotation config) |
| merge-base | 7425e42 |
Around each build:
- A real channel. A
ChannelPluginloaded the documented way — an extension inQWEN_HOME/extensionswith achannelsentry inqwen-extension.json. It subclasses the tree's ownChannelBase, so the routing, gating and rotation code under test is the real thing; the only fake part is the transport (WebSocket to a local fake chat platform). It reports the session ID the router handed it for every turn, which is what makes rotation observable from the chat side. - A real launcher.
qwen channel start <name>/qwen channel start(all channels) for the standalone legs, andqwen serve --workspace … --channel probe-botfor the daemon legs — the deployment shape the reported DingTalk bug came from. - A recording model server. A local OpenAI-compatible server that logs every request's full
messagesarray and answers deterministically with what it can see:CONTEXT_USER_MSGS=<n> | SECRET=<value|NONE>. So the bot's own reply is the assertion — after a rotation it literally cannot seeSECRET-ALPHA1any more. Host-issued side queries (next-turn suggestions) are tagged and excluded from turn counts. - A tap on the ACP wire. A shim at
argv[1]re-spawns the real bundle for the--acpchild and tees the JSON-RPC in both directions, soqwen/control/session/closefor a retired session is visible as a raw protocol frame rather than inferred. - Isolated
QWEN_HOME+ workspace per leg; macOS 26.6, Node v24.18.1.
Results
| # | Claim | Result | Evidence |
|---|---|---|---|
| 1 | Unconfigured channels behave exactly as today | ✅ | merge-base: 6 messages, one session, prompt grows every turn. PR build, channel with no bound: 5 messages, one session, and its sessions.json entry has no turns/startedAt — byte-identical shape to before |
| 2 | maxTurns rotates at the bound |
✅ | maxTurns: 3 → msgs 1-3 on session 9c8e2f9f, msg 4 on 10d92d2f; the fresh session answers SECRET=NONE |
| 3 | Only the route that hit the bound rotates | ✅ | alice rotated while bob's route on the same channel kept 842e969a |
| 4 | A channel without a bound is unaffected by one that has it | ✅ | plain-bot on the same router, same process: 5 messages, one session, no counter fields |
| 5 | maxAgeHours rotates on elapsed time |
✅ | maxAgeHours: 0.0084 (~30 s): +5 s reused, +37 s rotated |
| 6 | An age-only bound costs no per-message write | ✅ | across 4 messages the store's mtime only moves at session creation and at rotation, never per message, and no turns key is ever written |
| 7 | Counters persist; a daemon restart cannot reset the bound | ✅ | daemon leg: turns=2 on disk → daemon killed → reboot restores the route (Restored 1 dormant route(s)) and the same session with its history (CONTEXT_USER_MSGS=3) → msg 4 rotates. Counter resumed at 3, not 1 |
| 8 | Stores written before this change load cleanly and start their clock at the next message | ✅ | in-place upgrade: merge-base daemon wrote a pre-PR entry (3 msgs), PR daemon restored it, kept serving the same session with full context, counted from 1 and rotated on the 3rd message after upgrade |
| 9 | Rotation is announced in the chat | ✅ | notice is delivered before the answer from the fresh session in every rotation observed |
| 10 | The retired session is actually discarded | ✅ | ACP frame qwen/control/session/close {sessionId: 9c8e2f9f-…} right after the rotation log line |
| 11 | Rotation defers while a turn is running | ✅ | maxTurns: 2, msg 2's turn held open for 24 s; msg 3 arrived mid-turn at the bound and reused the session, msg 4 rotated |
| 12 | Bad bounds are rejected at parse time | ✅ | maxTurns: 0, maxTurns: -5, maxAgeHours: "daily", sessionRotation: "daily" all exit 1 with a field-accurate message; sessionRotation: null and {maxTurns: 3} start normally |
Both suites named in the description also pass on the head tree here: packages/channels/base SessionRouter.test.ts + ChannelBase.test.ts → 707 passed; packages/cli config-utils + start + daemon-worker + channel-settings-store → 232 passed.
Evidence
Before / after on the same harness — the bot's reply is the assertion: after rotation it no longer knows SECRET.
Daemon-managed channel — restart persistence and an in-place upgrade over a pre-PR route store.
The age bound, the mid-turn guard, and config validation.
Observations (non-blocking)
1. The age clock of a route that predates the upgrade is memory-only, so restarts can defer an age-only bound indefinitely.
shouldRotate() stamps toStartedAt for a restored entry that has none, but does not persist it; with an age-only bound countTurn() returns early, so nothing else writes the store either. Every daemon boot therefore re-arms the clock for that route.
Measured: a route created by the merge-base build, then given maxAgeHours ≈ 30 s. Three boots spanning 74 s of wall clock — each boot shorter than the bound — never rotated, and startedAt never appeared on disk. The control boot that stayed up 35 s rotated as designed.
Scope is narrow — pre-PR route entries only, age-only bounds, and only until the first rotation (after which the new session's startedAt is seeded and persisted) — and a channel with more than one active route gets the value written out incidentally by another route's persist. But it is exactly the shape of the reported case: one long-lived thread, a daemon that restarts on deploys. A this.persist() next to the this.toStartedAt.set(sessionId, Date.now()) in shouldRotate() closes it, at the cost of one write per session, once.
2. Context, not a defect: in qwen channel start the route store is write-only.
startSingle/startAll never restore it at boot (restoreSessions() is reached only from bridge crash recovery), and clearAll() on SIGINT deletes the file outright. I confirmed this is identical on the merge-base build, so it is not from this PR — but it does mean the persistence guarantee applies to daemon-managed channels specifically, which is where I tested it (row 7). Worth keeping in mind if anyone reads "a daemon restart cannot reset a bound" as covering channel start too.
3. Not covered here. Carrying counters across a reload that returns a new session ID (the daemon's reload returned the same ID in every run I got), rotation skipped while a session creation is in flight, and the qwen serve channel-settings validation path — all three are covered by the unit tests, just not by this run.
中文版
验证报告 — sessionRotation 真实环境跑通
我把 PR 两侧都构建出来，各自跑了一条真实的频道链路，而不是只读测试。描述里的每一条行为声明都成立，包括第三个 commit 新增的部分（聊天内提示、退役会话回收、回合进行中推迟轮换）。文末两点观察值得看一眼，但都不阻塞合入。
结论：行为与描述一致，可以合入。 唯一建议先修的是观察 (1)，一行 persist() 即可。
怎么验的
两棵干净的树，各自 npm ci && npm run build && npm run bundle：PR head 6dbca59，merge-base 7425e42。
- 真实频道：按官方文档的方式，用扩展（
QWEN_HOME/extensions里带channels字段的qwen-extension.json）加载一个ChannelPlugin。它继承所在树自己的ChannelBase，所以被测的路由、门禁、轮换逻辑都是真的，只有传输层（连本地假聊天平台的 WebSocket）是假的。它会把路由器为每一轮分配的 session ID 一并上报，轮换因此在"聊天侧"可见。 - 真实启动方式：独立腿用
qwen channel start，守护进程腿用qwen serve --workspace … --channel probe-bot——也就是这个 bug 最初被发现的部署形态。 - 记录型模型服务：本地 OpenAI 兼容服务，把每次请求完整的
messages落盘，并按它实际看到的内容确定性作答：CONTEXT_USER_MSGS=<n> | SECRET=<值|NONE>。于是机器人自己的回复就是断言——轮换之后它确实看不见SECRET-ALPHA1了。ACP host 发起的旁路请求（下一句建议）会被标记并排除在回合统计外。 - ACP 线协议探针：在
argv[1]放一个 shim，--acp子进程由它转发真实 bundle 并双向抓取 JSON-RPC，因此退役会话的qwen/control/session/close是原始协议帧，而非推断。 - 每条腿独立的
QWEN_HOME与 workspace；macOS 26.6，Node v24.18.1。
结果
| # | 声明 | 结果 | 证据 |
|---|---|---|---|
| 1 | 不配置时行为与今天完全一致 | ✅ | merge-base：6 条消息、一个会话、prompt 逐轮增长。PR 构建下未配置限度的频道：5 条消息一个会话，sessions.json 条目没有 turns/startedAt，磁盘结构与改动前一致 |
| 2 | maxTurns 在限度处轮换 |
✅ | maxTurns: 3 → 第 1-3 条在 9c8e2f9f，第 4 条换到 10d92d2f，新会话回答 SECRET=NONE |
| 3 | 只有触达限度的路由轮换 | ✅ | alice 轮换时，同频道 bob 的路由仍是 842e969a |
| 4 | 未配置限度的频道不受影响 | ✅ | 同一路由器、同一进程里的 plain-bot：5 条消息一个会话，无计数字段 |
| 5 | maxAgeHours 按时间轮换 |
✅ | maxAgeHours: 0.0084（约 30 秒）：+5 秒复用，+37 秒轮换 |
| 6 | 纯年龄限度没有每条消息的写盘 | ✅ | 4 条消息期间 store 的 mtime 只在会话创建和轮换时变动，逐条消息不写，且从不写 turns |
| 7 | 计数持久化，守护进程重启不能重置限度 | ✅ | 守护进程腿：磁盘 turns=2 → 杀掉进程 → 重启后恢复路由（Restored 1 dormant route(s)）并带着历史复用同一会话（CONTEXT_USER_MSGS=3）→ 第 4 条轮换。计数从 3 继续，不是从 1 |
| 8 | 旧版本写的存储能干净加载，从下一条消息开始计时 | ✅ | 原地升级：merge-base 守护进程写下 pre-PR 条目（3 条消息），PR 守护进程恢复它、带完整上下文继续服务、从 1 开始计数，并在升级后第 3 条消息轮换 |
| 9 | 轮换会在聊天里发提示 | ✅ | 观察到的每次轮换，提示都先于新会话的回答送达 |
| 10 | 退役会话真的被回收 | ✅ | 轮换日志之后紧跟 ACP 帧 qwen/control/session/close {sessionId: 9c8e2f9f-…} |
| 11 | 回合进行中不轮换 | ✅ | maxTurns: 2，第 2 条的回合被挂住 24 秒；第 3 条在回合进行中到达且已达限度，复用了会话，第 4 条才轮换 |
| 12 | 非法限度在解析期报错 | ✅ | maxTurns: 0、maxTurns: -5、maxAgeHours: "daily"、sessionRotation: "daily" 均以 1 退出并给出字段级信息；sessionRotation: null 与 {maxTurns: 3} 正常启动 |
描述里点名的两个套件在本机 head 树上也全绿：packages/channels/base 的 SessionRouter.test.ts + ChannelBase.test.ts 共 707 条通过；packages/cli 的 config-utils + start + daemon-worker + channel-settings-store 共 232 条通过。
观察（不阻塞）
1. 升级前就存在的路由，其年龄时钟只存在内存里，反复重启可以无限期推迟纯年龄限度。
shouldRotate() 会给没有起始时间的恢复条目盖上 toStartedAt，但不持久化；而纯年龄限度下 countTurn() 直接返回，也没有别的地方写盘。于是每次守护进程启动都会把这个路由的时钟重新归零。
实测：先用 merge-base 构建产生一个路由，再配上 maxAgeHours ≈ 30 秒。三次启动横跨 74 秒真实时间（每次在线时长都短于限度），从未轮换，startedAt 也始终没落盘；作为对照，一次在线 35 秒的启动按预期轮换了。
适用范围有限——只影响改动前写下的路由条目、只在纯年龄限度下、且只到第一次轮换为止（之后新会话的 startedAt 会被写入）；另外，频道里若有多个活跃路由，这个值会被别的路由的写盘顺带带出去。但这恰好就是报告场景的形状：一个长期存在的 thread，加上会随发布重启的守护进程。在 shouldRotate() 里 this.toStartedAt.set(sessionId, Date.now()) 旁边补一次 this.persist() 即可，代价是每个会话多写一次盘。
2. 背景说明，不是缺陷：qwen channel start 的路由存储实际上只写不读。
startSingle/startAll 启动时从不恢复它（restoreSessions() 只在 bridge 崩溃恢复路径上被调用），并且 SIGINT 时 clearAll() 会直接删除该文件。我在 merge-base 构建上确认行为完全相同，所以这不是本 PR 引入的——但这意味着持久化保证具体是针对守护进程托管的频道，我也正是在那里验证的（第 7 行）。如果有人把"守护进程重启不会重置限度"理解成也覆盖 channel start，需要注意这一点。
3. 本次未覆盖：重载返回新 session ID 时计数的迁移（几次运行里守护进程重载都返回了同一个 ID）、会话创建在途时跳过轮换检查、以及 qwen serve 的频道设置校验路径——这三点单测有覆盖，只是本次真实链路没跑到。
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 59 passed · 1 failed · 60 total 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：59 通过 · 1 失败 · 60 总计 Verification reportPR #8927 Deep Verification —
|
| Cell | Scenario | Head | Base (control) |
|---|---|---|---|
| A | 5 msgs, one route, maxTurns: 2 |
[1,1,2,2,3] — 3 sessions, rotates at msg 3 and 5 ✅ |
1 session forever; no rotation API exists ✅ (expected-fail control) |
| B | sibling route under its bound (maxTurns: 3, 2 msgs) |
only the bounded route rotates; sibling keeps its session and is not discarded ✅ | — |
| C | unbounded channel next to bounded | unbounded: 1 session across 5 msgs; bounded rotates ✅ | — |
| D | maxAgeHours: 2, fake Date.now |
no rotate at 1h59m; rotates at 2h1m ✅ | — |
| E | retirement machinery | listener fired once with retired id + target; discardSession('sess-1'); sanitized stderr log emitted ✅ |
— |
| F | bounds 0 / -3 / NaN |
dropped defensively, no per-message rotation ✅ | — |
Count: head 15/15, base 2/2. Witness: 01-ab-rotation-head-vs-base.png.
Secondary claim 1 — persistence and store shape (harness/persistence.mjs): head 13/14, base 2/2. Witness: 02-persistence-restart-legacy-head-vs-base.png.
| Cell | Result (head) |
|---|---|
| G restart survival | turns persist; after restoreSessions() the 4th message rotates exactly at the bound; no duplicate creation ✅ |
| H store shape | unbounded-channel entries carry no turns/startedAt — byte-same shape as base (A/A shape control, passes on both arms); bounded entry gets turns: 1 at creation, no startedAt without maxAgeHours ✅ |
| I legacy store | restores, first message reuses the legacy session (no rotate-on-sight), rotates after post-upgrade turns reach the bound ✅ |
| J fresh age clock | startedAt persisted at creation → rotation fires across a restart ✅ |
| K legacy age clock | FAIL — Finding 1 below ❌ |
| L1/L2/L3 hand-edited stores | turns: 1e308 → at most one rotation then normal cadence; turns: "lots" → entry rejected, store rewritten, fresh session; turns: -5 → defers, never per-message-rotates ✅ |
Secondary claim 2 — safe retirement paths (harness/lazy-and-defer.mjs): head 10/10, base 2/2. Witness: 03-lazy-reload-defer-head-vs-base.png.
- M: evicted (non-live) route at its bound rotates without a
loadSessionattempt — the bound cannot be dodged by memory eviction. - N: reload that returns a new ID carries
turns/startedAtover; rotation then lands exactly at the bound. - O: rotation defers while the activity checker reports the session active, enforces on the next message once settled; clearing the checker re-enables.
- P: two concurrent messages share an in-flight creation (no invalidation); the bound applies to the next message.
- Q (ad-hoc probe,
logs/03b-lazy-restoreRoutes-probe.txt): the lazyrestoreRoutes()path used bydaemon-worker(recoveryMode: 'lazy') also carries restored turn counts — m3 reuses the restored session, m4 rotates (LAZY-RESTART-BOUND: PASS).
Corrections (to the PR description, not code requests)
- "One wiring line in each of
start.tsanddaemon-worker.ts… exactly three call sites" (Risk & Scope) — stale for the final head. At6dbca5908fthe registration is centralized in theChannelBaseconstructor (setChannelRotationhas exactly one production call site,ChannelBase.ts:855);start.ts/daemon-worker.tscontain nosessionRotationreferences. Verified this is behaviorally equivalent or better: all three launch paths (start.ts:369,start.ts:498single-channel,daemon-worker.ts:539) construct channels viacreateChannel→ChannelBaseconstructor, and the M8 mutation proves the registration is load-bearing (removing it kills 4 tests). The consolidation happened in commit53a6777("register sessionRotation bounds in every launch mode"); the scope note describes the earlier per-site wiring. - "No user-facing notice is posted to the chat when a rotation happens — the reset is silent" (Risk & Scope, both languages) — contradicted by the final code:
handleSessionRotatedsends"This conversation reached its configured limit and was rotated; starting a fresh session."to the affected chat/thread, and the updated docs say the same. The notice is pinned by test (M6 mutation killsannounces rotation and discards the retired session). The docs and code agree with each other; only the description lags.
Findings
Finding 1 — Low — legacy sessions under maxAgeHours-only lose their age clock on daemon restart
Repro (preserved harness): node harness/persistence.mjs packages/channels/base/dist HEAD <scratch> — cell K. A pre-rotation store entry (sessionId only, no startedAt) on a channel configured with { maxAgeHours: 2 } alone: the first post-upgrade message seeds startedAt in memory inside shouldRotate() and returns without persisting; countTurn is a no-op without maxTurns, and nothing else on the reuse path writes. The on-disk entry after the message:
{"sessionId":"legacy-aged","target":{...},"cwd":"/cwd"} // startedAt absentConsequence: each daemon restart re-seeds the clock at the first message, so age rotation for this cohort requires maxAgeHours of continuous uptime. This deviates from the description ("Turn counts and start times persist alongside the routes, so a daemon restart cannot reset a bound") and the docs ("Counters … survive a daemon restart"). Bounds established: (a) fresh sessions are unaffected — their startedAt persists at creation (cell J passes); (b) turn-bound channels are unaffected — countTurn persists every message; (c) the window closes on any unrelated persist — an unrelated bounded route's write flushed the seeded clock in the same run (cell K.control passes). Blast radius is the one-time migration cohort on age-only configs. Note the PR's own test suite is green on both sides of this axis — nothing pins it (see mutation note below).
Suggested fix (measured, preserves commit intent):
if (startedAt === undefined) {
this.toStartedAt.set(sessionId, Date.now());
+ this.persist();
return false;
}Applied in a scratch rebuild: the persistence harness flips to 14/14 (hostile fixture clean), the full SessionRouter + ChannelBase suite stays 707/707 (benign fixtures byte-identical — in particular does not write per message when only maxAgeHours is configured still passes, because the seed branch fires only for legacy sessions, not per message), logs/06-kfix-persistence.txt, logs/06-kfix-suite.txt. Since the suite is green with and without the patch, the fix should ship with its pinning fixture, e.g. "legacy store + maxAgeHours: after the first routed message the persisted store contains startedAt, and the age bound survives a restart".
Finding 2 — Nit — description-vs-code drift (see Corrections)
The two description statements above contradict the final head. No code change requested; flagged so the next reader does not rely on "silent rotation" or the three-call-site topology.
Vacuity check and mutation matrix
Baseline SessionRouter.test.ts + ChannelBase.test.ts: 707/707 green. Positive control: M1's off-by-one turned exactly the rotation block red with behavioral assertions (expected 'session-2' to be 'session-1'-style), proving the harness can fail the suite. Witness: 04-mutation-matrix-11-of-11-killed.png, raw logs under logs/mutations/.
| # | Mutant (guard removed/broken) | Result | Killed by |
|---|---|---|---|
| M1 | >= → > in shouldRotate |
KILLED (9) | all maxTurns rotation tests + announces rotation and discards the retired session |
| M2 | drop persist() in countTurn |
KILLED (2) | persists turn counts so a restart cannot reset the bound, persists once per routed message instead of stacking writes |
| M3 | drop isSessionActive defer condition |
KILLED (2) | both defers rotation… tests (router + ChannelBase) |
| M4 | drop counter migration on ID-changing reload | KILLED (1) | carries counters over an ID-changing reload |
| M5 | rotation check disabled (false) |
KILLED (12) | entire rotation block, both packages |
| M6 | ChannelBase: no onSessionRotated subscription |
KILLED (1) | announces rotation and discards the retired session |
| M7 | ChannelBase: activity checker always false | KILLED (1) | defers rotation while the outgoing turn is still running |
| M8 | ChannelBase: no setChannelRotation registration |
KILLED (4) | both registration tests + announce + defer |
| M9 | cli: sessionRotation parse dropped |
KILLED (3) | parses sessionRotation bounds, both throw-tests |
| M10 | settings-store validation disabled | KILLED (1) | accepts env-resolvable descriptor fields and typed shared fields |
| M11 | legacy seed branch rotates on sight | KILLED (1) | accepts route stores written before rotation existed |
11/11 killed, zero survivors. Every guard the PR introduces is pinned by a behavioral assertion, and each failure quoted the expected-vs-actual mismatch (no import/compile-break reds). The reverse mutation (Finding 1's fix) left the suite green on both sides — the unpinned axis is exactly where Finding 1 lives; the fixture that would go red is named there.
Config-parse surface (harness/config-parse.mjs against the real cli dist/): 15/15 — accepts maxTurns-only / maxAgeHours-only / both / fractional / {}→unset / null→unset / omitted→unset; rejects 0, -1, NaN, Infinity, "5", maxAgeHours: 0, non-object, and arrays, each with a sessionRotation-named error. The two validation sites are equivalent: isValidRotationBound (channel-base) vs the settings-store inline expression agree on all 15 ladder values. Witness: 05-config-parse-reject-accept-matrix.png.
Targeted gates
| Gate | Result |
|---|---|
packages/channels/base full vitest suite |
1039/1039 passed (19 files) |
packages/cli channel + settings-store suites (config-utils, start, daemon-worker, channel-settings-store) |
232/232 passed (4 files) |
cli workspace typecheck (tsc --noEmit) |
0 errors |
| ESLint on the 6 changed production files | clean, liveness-proven: a planted const unusedPlantedVar = 1; in types.ts was reported (no-unused-vars), then removed |
No pre-existing failures encountered on either arm; no repo-wide gate was run (see Not covered).
Not covered
- Per-commit attribution: the checkout is depth-2 (
is-shallow-repository: true);git rev-list HEAD^1..HEAD^2yields 1 commit locally while the metadata snapshot lists 5. The aggregateHEAD^1..HEADdiff is what was verified; the intermediate states (e.g. the per-site wiring of53a6777) were not individually exercised. - End-to-end with a real daemon and real chat platform: rotation's announcement/discard wiring was exercised at the
ChannelBaseunit level (its own tests, mutation-pinned) and at the router level with a fake bridge — this reproduces the shape of the wire flow, not delivery through a live platform adapter. Thestart.ts/daemon-worker.tsruntime paths were verified by their suites + code trace, not by booting a daemon. - Repo-wide gates: only the two affected workspaces were run (per scope). No repo-wide
npm run test, fullnpm run lint, or integration suites. - Token-based bound: explicitly out of scope per the PR description; not probed.
- Base-side
HEAD^1differs from the metadatabaseRefOid(7425e42f…): the merge ref was rebuilt against a newer main tip (962dc8eadc, including fix(serve): Keep restore request shapes distinct #8933). Per the CI contract the merge ref is authoritative; the diff between the two main tips is outside this PR. - A/B for announcement ordering UX (notice posts before the successor's reply) — code-read only.
Methodology
Environment: the CI verify container (node v22.23.2, Linux, $RUNNER_TEMP=/__w/_temp), merge-ref checkout at c82232f6; npm ci + npm run build pre-run. Harnesses (harness/*.mjs) import the compiled dist/ of each arm by absolute path and drive it with a fake bridge peer encoding the real peer semantics (fresh-ID newSession, echo loadSession, recorded discardSession) — no stubbing of the unit under test; time was controlled by patching Date.now around the aged cells. Base control: scratch worktree at HEAD^1, only packages/channels/base rebuilt against the root node_modules (no internal workspace links in this package's closure, verified). Cell K's probe encodes the PR's own stated invariant, so its red counts as a fail. Mutations were applied with point edits to source, run under vitest against source, and restored with git checkout (verified git status --porcelain empty after each). Raw per-arm logs: logs/01-…–logs/06-… and logs/mutations/.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
🤖 Addressed the latest review feedback (round 4/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/10 轮）。改动内容与我反驳保留之处如下： Autofix review round for PR #8927This round triaged the real-stack verification report from @wenshao (issue-level comment; no inline comments, no failed checks, no base conflicts). One observation was addressed with a code fix; the other two were context-only and are declined with reasons below. Feedback dispositions1. Observation (1) — age clock of a pre-upgrade route is memory-only, restarts can defer an age-only bound indefinitely → Addressed. New regression test: 2. Observation (2) — 3. Observation (3) — paths not exercised by the real-stack run → No action needed. Changes
Commit: Verification
Integration tests were not run: the touched behavior is exercised directly by the package's unit tests, not only through the bundled CLI or integration harness. 中文说明PR #8927 的 Autofix 评审轮次本轮分诊了 @wenshao 的真实环境验证报告（issue 级评论；没有行内评论、没有失败的检查、没有与 base 的冲突）。其中一条观察以代码修复处理；另外两条属于背景说明，附理由予以婉拒。 反馈处理结果1. 观察 (1) —— 升级前就存在的路由，其年龄时钟只在内存里，反复重启可以无限期推迟纯年龄限度 → 已修复。 新增回归测试： 2. 观察 (2) —— 3. 观察 (3) —— 本次真实链路未覆盖的路径 → 无需处理。 改动内容
提交： 验证
未运行集成测试：本次触及的行为由包的单测直接覆盖，并非只能通过打包后的 CLI 或集成测试框架验证。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 10 this bot stops and leaves the PR for a human. · 有空请复审；第 10 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
e4f78f3
|
@qwen-code /resolve |
|
Qwen Code attempted to resolve merge conflicts but the run did not complete successfully. Check the workflow run for full logs. |
…rotation Resolve six conflicts; this branch's sessionRotation is additive over main's multiSession/named-tasks work, so most hunks are a union: - docs overview: keep both option rows and both sections. - AcpBridge.loadSession: keep this branch's settleOnChildExit wrapper around main's unstable_resumeSession call. - ChannelBase: keep both fields, both constructor blocks (main's multiSession validation then this branch's rotation wiring), and track the turn while awaiting it (`await current`) — awaiting main's `tracked` instead defers releaseQueuedTurn past the caller and breaks the collect-mode rotation count; both suites pass this way. - SessionRouter: union of imports, fields and methods; the restore loop keeps this branch's shape plus main's liveSessionIds bookkeeping. - types.ts / config-utils.ts: keep both config fields. tsc clean for channels/base and cli; channels/base 1209 passed, cli channel commands 365 passed.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 5)": none — full chunk read (diff lines 3133–3492, un-truncated) and all source cross-checks completed; removeSessionId 's missing rotationDeltas cleanup was exam….
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 10, not a blocker) — recorded, not requested in this round:
docs/users/features/channels/overview.md:66 — [review] R10-4 duplicate stale sessionScope row contradicts the existing one (hides chat_thread)packages/channels/base/src/SessionRouter.ts:681 — [review] R10-7 lazy-reload fallback replacement seeding ungated by any test (mutant survives 782/782)packages/channels/base/src/ChannelBase.test.ts:13383 — [review] R10-8 failed-shell rotation cleanup (catch branch) pinned by no test (mutant leaves 627/627 green)packages/channels/base/src/ChannelBase.test.ts:13470 — [review] R10-11 lazy death/revival rotation-state survival pinned by no testpackages/channels/base/src/ChannelBase.ts:2372 — [review] R9-2 comment claims /clear is the only sessionQueues deleter; rotation also deletes itpackages/channels/base/src/ChannelBase.ts:2412 — [review] R9-3 sessionPendingTurns map has no purge sitepackages/channels/base/src/SessionRouter.test.ts:2440 — [review] R9-4 no test clears a route inside an overlapping restore's post-reservation windowdocs/users/features/channels/overview.md:136 — [review] R9-5 documented single-scope rotation notice semantics have zero test coverage
Convergence: round 10 posted 4 inline comment(s), 3 of them reported for the first time. Findings keep coming back to the same files: docs/users/features/channels/overview.md (findings in round 9; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
[Critical] R8-1: Overlapping restoreSessions() rewind LIVE rotation counters to the stale persisted snapshot when the later restore's reservation pass captures no live state (route wiped by the earlier restore, load unsettled): the load settle re-seeds toTurns/toStartedAt from the snapshot unconditionally via restoreRotationState (SessionRouter.ts:1168), made durable by the last finisher's flush. Independently re-derived this round by two review agents and probe-reproduced at this head: live counter 3 after waiter, final in-memory/persisted 2 (expected 3); a one-line no-rewind guard flips it to 3/3 with all 155 SessionRouter tests green under the guard. QQChannel fires restoreSessions() on cold-start READY and re-arms coldStart on abnormal WS close / INVALID_SESSION, so the overlap is production wiring. Fix: make live state win at load completion — skip restoreRotationState when the session already has live counters, or re-capture live rotation state when the load completes and net it via carryLiveRotationState. Acceptance test: a variant of SessionRouter.test.ts:2214 where the second restore starts before the first resolves the key's loadSession and a message routes in between — final toTurns (and persisted turns) must equal snapshot+1; the test goes red without the guard.
[Critical] R8-4: The tombstone mechanism (suspendedDeletionKeys) guards only the reservation pass (SessionRouter.ts:1103) — a /clear landing after both reservation passes invalidates only the LATER restore's operation (invalidateRouteOperation reaches only creatingSessions.get(key)), so the EARLIER restore's orphaned operation settle passes assertOperationCurrent and re-adds the cleared route; the settle path (:1143-1185) never re-checks suspendedDeletionKeys. Re-asserted by code read at this head; unchanged since round 8. Fix: re-check suspendedDeletionKeys at load settle, and/or invalidate the superseded restore operation so the earlier settle cannot resurrect a cleared key.
[Critical] R8-10: The waiter retry heuristic (SessionRouter.ts:487-495) classifies any invalidation with a successor as a rotation/reload handoff; /clear (removeSession) followed immediately by a new message produces the same shape, so a message parked on a restore reservation at clear time retries into the fresh post-clear session instead of being dropped — /clear defeated for that message, and ChannelBase's generation guard cannot catch it (generation snapshot at enqueue, after resolve returns). Probe-reproduced in round 8: removedIds at /clear [], parked waiter resolved to the post-clear session; reverting to the unconditional throw flips it. Mechanism unchanged at this head (re-read). Fix: distinguish removeSession invalidations from rotation/reload handoffs so deliberate rejections stay terminal even when a successor exists.
[Critical] R8-11: The overlap-restore carry protects only ROTATION state (liveRotation = turns/startedAt/leases); a live toTarget mutation — promoteTargetToGroup's monotonic isGroup promotion — made during the suspension window is wiped by the later restore's reservation pass (deleteByKey) and re-seeded from the stale snapshot at settle (toTarget.set(sessionId, entry.target)), then made durable by the last finisher's flush. Re-asserted by code read at this head; unchanged since round 8. Fix: carry the live toTarget across the wipe window as well (or skip the snapshot re-seed when a live target existed).
[Critical] R8-17: The persisted-entry validation-drop path (persisted.droppedKeys -> deleteByKey, SessionRouter.ts:1085-1087) runs BEFORE persistSuspendDepth++ and never tombstones — an overlapping restore reading the same stale snapshot re-applies the drop and wipes a live replacement route created mid-window (deleteByKey never calls discardSession, so the replacement session also leaks on the bridge). Re-asserted by code read at this head; unchanged since round 8. Fix: tombstone validation-dropped keys while a restore is (or may be) in flight, and discard the replaced session.
[Critical] R6-1 (re-check, narrowed): the persist suspension opened by restoreSessions() still has no timeout/lifecycle cleanup at this head — persistSuspendDepth is released only in that call's finally, the load loop has no timeout, and QQChannel's cold-start READY restore is fire-and-forget on the long-lived shared router (coldStart re-arms on non-1000 WS close / INVALID_SESSION, so restores overlap). A wedged-but-alive ACP child whose session/load never responds (async I/O hang, no exit, no event-loop stall — neither the exit-reject nor the stall watchdog fires) pins persistSuspendDepth >= 1 for the router's lifetime: every later persist() is silently dropped and the on-disk route store goes permanently stale until restart. Probe-traced mechanism in rounds 6-9; re-asserted by code read at this head. Fix: a restore-level timeout/lifecycle guard that fails the wedged loads and releases the suspension.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 5)"：none — full chunk read (diff lines 3133–3492, un-truncated) and all source cross-checks completed; removeSessionId 's missing rotationDeltas cleanup was exam…。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 10 轮，非阻断）——已记录，本轮不要求修改：共 8 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 10 轮发布了 4 条行内评论，其中 3 条是首次提出。发现反复回到同一批文件：docs/users/features/channels/overview.md（第 9 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
[Critical] R8-1: Overlapping restoreSessions() rewind LIVE rotation counters to the stale persisted snapshot when the later restore's reservation pass captures no live state (route wiped by the earlier restore, load unsettled): the load settle re-seeds toTurns/toStartedAt from the snapshot unconditionally via restoreRotationState (SessionRouter.ts:1168), made durable by the last finisher's flush. Independently re-derived this round by two review agents and probe-reproduced at this head: live counter 3 after waiter, final in-memory/persisted 2 (expected 3); a one-line no-rewind guard flips it to 3/3 with all 155 SessionRouter tests green under the guard. QQChannel fires restoreSessions() on cold-start READY and re-arms coldStart on abnormal WS close / INVALID_SESSION, so the overlap is production wiring. Fix: make live state win at load completion — skip restoreRotationState when the session already has live counters, or re-capture live rotation state when the load completes and net it via carryLiveRotationState. Acceptance test: a variant of SessionRouter.test.ts:2214 where the second restore starts before the first resolves the key's loadSession and a message routes in between — final toTurns (and persisted turns) must equal snapshot+1; the test goes red without the guard.
[Critical] R8-4: The tombstone mechanism (suspendedDeletionKeys) guards only the reservation pass (SessionRouter.ts:1103) — a /clear landing after both reservation passes invalidates only the LATER restore's operation (invalidateRouteOperation reaches only creatingSessions.get(key)), so the EARLIER restore's orphaned operation settle passes assertOperationCurrent and re-adds the cleared route; the settle path (:1143-1185) never re-checks suspendedDeletionKeys. Re-asserted by code read at this head; unchanged since round 8. Fix: re-check suspendedDeletionKeys at load settle, and/or invalidate the superseded restore operation so the earlier settle cannot resurrect a cleared key.
[Critical] R8-10: The waiter retry heuristic (SessionRouter.ts:487-495) classifies any invalidation with a successor as a rotation/reload handoff; /clear (removeSession) followed immediately by a new message produces the same shape, so a message parked on a restore reservation at clear time retries into the fresh post-clear session instead of being dropped — /clear defeated for that message, and ChannelBase's generation guard cannot catch it (generation snapshot at enqueue, after resolve returns). Probe-reproduced in round 8: removedIds at /clear [], parked waiter resolved to the post-clear session; reverting to the unconditional throw flips it. Mechanism unchanged at this head (re-read). Fix: distinguish removeSession invalidations from rotation/reload handoffs so deliberate rejections stay terminal even when a successor exists.
[Critical] R8-11: The overlap-restore carry protects only ROTATION state (liveRotation = turns/startedAt/leases); a live toTarget mutation — promoteTargetToGroup's monotonic isGroup promotion — made during the suspension window is wiped by the later restore's reservation pass (deleteByKey) and re-seeded from the stale snapshot at settle (toTarget.set(sessionId, entry.target)), then made durable by the last finisher's flush. Re-asserted by code read at this head; unchanged since round 8. Fix: carry the live toTarget across the wipe window as well (or skip the snapshot re-seed when a live target existed).
[Critical] R8-17: The persisted-entry validation-drop path (persisted.droppedKeys -> deleteByKey, SessionRouter.ts:1085-1087) runs BEFORE persistSuspendDepth++ and never tombstones — an overlapping restore reading the same stale snapshot re-applies the drop and wipes a live replacement route created mid-window (deleteByKey never calls discardSession, so the replacement session also leaks on the bridge). Re-asserted by code read at this head; unchanged since round 8. Fix: tombstone validation-dropped keys while a restore is (or may be) in flight, and discard the replaced session.
[Critical] R6-1 (re-check, narrowed): the persist suspension opened by restoreSessions() still has no timeout/lifecycle cleanup at this head — persistSuspendDepth is released only in that call's finally, the load loop has no timeout, and QQChannel's cold-start READY restore is fire-and-forget on the long-lived shared router (coldStart re-arms on non-1000 WS close / INVALID_SESSION, so restores overlap). A wedged-but-alive ACP child whose session/load never responds (async I/O hang, no exit, no event-loop stall — neither the exit-reject nor the stall watchdog fires) pins persistSuspendDepth >= 1 for the router's lifetime: every later persist() is silently dropped and the on-disk route store goes permanently stale until restart. Probe-traced mechanism in rounds 6-9; re-asserted by code read at this head. Fix: a restore-level timeout/lifecycle guard that fails the wedged loads and releases the suspension.
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
@qwen-code /resolve |
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge conflict resolution — PR #8927 (
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not complete locally (packages/cli vitest run timed out at its full deadline — infrastructure; the 7 failing files parsed before timeout were all outside the diff).
Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies.
Test Plan (not a blocker): 1023 tests passed — this review observed 1300, 26, 501, 303, 280, 208, 60, 4, 305, 137, 92 passed; 158 tests passed — this review observed 1300, 26, 501, 303, 280, 208, 60, 4, 305, 137, 92 passed.
Deferred under the convergence posture (round 11, not a blocker) — recorded, not requested in this round; 1 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/channels/base/src/ChannelBase.ts:6386 — [review] Critical [fails-closed] [new-surface] D11-1 the /btw no-turn path returns after router.resolve() without uncountTurn/releaseRoutingLease — one /btw permanently leaks the routing leas…packages/channels/base/src/AcpBridge.ts:267 — [review] D11-2 applySessionApprovalMode's conn.setSessionMode runs outside settleOnChildExit — child exit mid-setSessionMode hangs the newSession/loadSession callerpackages/channels/base/src/AcpBridge.test.ts:1205 — [review] D11-3 child-exit test stubs connection.loadSession but production calls conn.unstable_resumeSession — dead stub, no in-flight resume request exercised(body) — [review] D11-4 PR description contradicts shipped behavior: claims rotation is silent, but handleSessionRotated posts an in-thread notice; repeats the hand-edit-routes.json recovery claim the issue triage correcteddocs/users/features/channels/overview.md:66 — [review] D11-5 added duplicate sessionScope table row contradicts the existing row (advertises legacy thread, omits chat_thread) — already recorded in round 10's deferral list
Convergence: round 11 posted 7 inline comment(s), 3 of them reported for the first time; the previous round posted 4 (3 new). Findings keep coming back to the same files: packages/channels/base/src/SessionRouter.ts (findings in round 10; 3 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not complete locally (packages/cli vitest run timed out at its full deadline — infrastructure; the 7 failing files parsed before timeout were all outside the diff).
未审查：反向审计——有审计 agent 运行并打开了自己的 brief，但没有 agent 是用 CLI 构建的 prompt 启动的——启动 prompt 是手写的，agent 实际被要求做的并不是本 skill 所认证的内容。
Test Plan（非阻断）：1023 tests passed — this review observed 1300, 26, 501, 303, 280, 208, 60, 4, 305, 137, 92 passed; 158 tests passed — this review observed 1300, 26, 501, 303, 280, 208, 60, 4, 305, 137, 92 passed。
收敛姿态下延后（第 11 轮，非阻断）——已记录，本轮不要求修改；其中 1 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 5 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 11 轮发布了 7 条行内评论，其中 3 条是首次提出；上一轮发布了 4 条（其中 3 条首次提出）。发现反复回到同一批文件：packages/channels/base/src/SessionRouter.ts（第 10 轮已出过发现，本轮又有 3 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.0)
…#8927) Resolve two conflicts: keep both parseSessionRotationConfig and the new optionalPlainStringField helper in config-utils, and rebuild the docs Options table keeping main's messagePrefix row plus the sessionRotation row (dropping the duplicate legacy sessionScope row). Also un-splice the Session Rotation section from the middle of Named Tasks and restore a valid JSON example (chat_thread instead of legacy thread), and document the multiSession incompatibility. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…nectivity hangs (#8927) The reservation pass in restoreSessions() superseded an in-flight create/load without invalidating it, so the superseded operation could still commit (orphaning a live session) or its settle could clobber the successor. It now invalidates first like removeSession/rotateRoute do: at most one restore ever settles a key, superseded settles discard instead of committing (skipping the discard when a successor owns the key — it is loading the same persisted session), and superseded creators re-route to the successor instead of failing the message. Wipe state (rotation counters, routing leases, group-promoted target, bridge-liveness) is now recorded per routing key so an overlapping restore landing inside the wipe window adopts it instead of rewinding to the stale disk snapshot; a failed load re-routes to the wiped session while in-flight messages still hold leases on it, and reclaims it otherwise. Validation drops are applied inside the persist suspension with tombstones, so an overlapping restore cannot re-apply a drop to a replacement route created mid-window. A child exit mid-restore used to fast-complete the restore and let the end flush prune every un-reached route from the persisted store. The bridge now throws BridgeConnectivityError for connection-level failures, and the restore aborts on it: un-attempted routes are kept in memory and on disk for the crash-recovery restore to retry. AcpBridge applies the session approval mode inside the settle window and wraps prompt() in it too, so a child exit mid-setSessionMode or mid-prompt rejects the caller instead of hanging it (and with it the pending-turn bookkeeping that gates rotation). ChannelBase refunds the resolve-time turn count for a rejecting shouldContinue and for /btw side questions, matching the other no-turn paths. Finally, sessionRotation is rejected together with multiSession at config parse and in the managed settings store: named tasks resolve sessions without consulting the rotation gate, so the bound would be accepted but never fire. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…counting (#8927) New pins for review-flagged blind spots: rotation is skipped while a reload is in flight on an at-bound route (fake timers expire the age bound mid-flight; the load is neither invalidated nor discarded); waiters that outlive an invalidation are counted and leased on the session they land on; an unregistered activity checker no longer gates rotation; and a non-object sessionRotation fails config parsing loudly. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…ttle (#8927) The per-key load-failure drop and forgetManagedSession removed routes without writing a suspended-deletion tombstone, so an overlapping restore reading the stale snapshot re-reserved the key and resurrected the removed route — in the failure case wiping a replacement session created mid-window. Both paths now tombstone like removeSession and rotateRoute, and the settle additionally re-checks the tombstone set after each load: a removal that lands after a restore reserved a key now fails that settle through the same invalidate-discard path as a /clear instead of resurrecting the route. Mutation-verified: dropping either tombstone or the settle re-check turns the new tests red. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@qwen-code /retry |
|
🔄 AutoFix re-armed. The next scan re-reads this PR's feedback from the start and the round counter resets. Nothing was deleted — this marker supersedes the evaluation markers above it. 中文说明🔄 已重新武装 AutoFix。下一次扫描会从头重新读取本 PR 的反馈，轮次计数也已重置。未删除任何内容 —— 本标记使其上方的评估标记失效。 |
qwen-code-dev-bot
removed
the
autofix/needs-human
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 3)": the ChannelBase.ts inbound span between the collect-buffer release ( :6681 ) and trackSessionTurn ( :7183 ) — ~500 lines I did not read end to end, so an ea…; "agent reverse-audit (round 2)": whether a failed bridge.prompt on a session the bridge already reported dead re-emits sessionDied in AcpBridge / DaemonChannelBridge (the self-healing pre…; "agent reverse-audit (round 1)": whether toCwd can diverge from the persisted entry.cwd for a managed worktree session (the capture carries target and rotation but not cwd , and named-s…; chunk 5: none — no check was cut short.; "agent reverse-audit (round 2)": did not read DaemonChannelBridge.loadSession (DaemonChannelBridge.ts:439) to confirm whether the daemon bridge's load can hang without settling, which is what…, and 3 more.
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
Test Plan (not a blocker): 1355 tests passed — this review observed 1360, 50, 28706, 508, 306, 288, 209, 61, 4, 319, 143, 97 passed; 472 tests passed — this review observed 1360, 50, 28706, 508, 306, 288, 209, 61, 4, 319, 143, 97 passed.
Deferred under the convergence posture (round 12, not a blocker) — recorded, not requested in this round; 1 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/channels/base/src/ChannelBase.ts:2717 — [review] Critical [fails-closed] [new-surface] Rotation discards a session with a /btw still in flight;…docs/users/features/channels/overview.md:150 — [review] Doc overstates what rotation clears: channel memory…packages/channels/base/src/AcpBridge.test.ts:1214 — [review] No test pins that the dead-child rejection is a…packages/channels/base/src/AcpBridge.test.ts:1237 — [review] The mid-approval-mode tests never reach the RPC they claim…packages/channels/base/src/ChannelBase.test.ts:14409 — [review] The thread-notice test cannot distinguish the two…packages/channels/base/src/ChannelBase.test.ts:14477 — [review] The plain-inbound deferral test silently runs on 'steer'…packages/channels/base/src/SessionRouter.test.ts:2138 — [review] Nothing pins that the rotation deferral is scoped per…packages/channels/base/src/SessionRouter.test.ts:2515 — [review] The settle skip-discard guard is unwitnessed, and a…packages/channels/base/src/SessionRouter.test.ts:3664 — [test] Duplicate unregister test is strictly weaker than the one…packages/channels/base/src/SessionRouter.ts:1368 — [review] The abort-vs-prune contract never engages in…packages/channels/base/src/SessionRouter.ts:1373 — [review] An aborted restore reports {restored: 0, failed: 0} and…packages/cli/src/commands/channel/config-utils.ts:130 — [review] The rotation !== null clause is live production…
Convergence: round 12 posted 4 inline comment(s), 4 of them reported for the first time; the previous round posted 7 (3 new). Findings keep coming back to the same files: packages/channels/base/src/SessionRouter.ts (findings in rounds 8, 11; 4 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 3)"：the ChannelBase.ts inbound span between the collect-buffer release ( :6681 ) and trackSessionTurn ( :7183 ) — ~500 lines I did not read end to end, so an ea…；"agent reverse-audit (round 2)"：whether a failed bridge.prompt on a session the bridge already reported dead re-emits sessionDied in AcpBridge / DaemonChannelBridge (the self-healing pre…；"agent reverse-audit (round 1)"：whether toCwd can diverge from the persisted entry.cwd for a managed worktree session (the capture carries target and rotation but not cwd , and named-s…；chunk 5：none — no check was cut short.；"agent reverse-audit (round 2)"：did not read DaemonChannelBridge.loadSession (DaemonChannelBridge.ts:439) to confirm whether the daemon bridge's load can hang without settling, which is what…，另有 3 条。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
Test Plan（非阻断）：1355 tests passed — this review observed 1360, 50, 28706, 508, 306, 288, 209, 61, 4, 319, 143, 97 passed; 472 tests passed — this review observed 1360, 50, 28706, 508, 306, 288, 209, 61, 4, 319, 143, 97 passed。
收敛姿态下延后（第 12 轮，非阻断）——已记录，本轮不要求修改；其中 1 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 12 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 12 轮发布了 4 条行内评论，其中 4 条是首次提出；上一轮发布了 7 条（其中 3 条首次提出）。发现反复回到同一批文件：packages/channels/base/src/SessionRouter.ts（第 8、11 轮已出过发现，本轮又有 4 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.0)
| if (this.suspendedDeletionKeys.has(key)) { | ||
| this.invalidateRouteOperation(key); | ||
| } |
There was a problem hiding this comment.
[Critical] R8-4: (fix-induced) [fails-closed] [regression] The settle-side tombstone re-check that closed R8-4 invalidates by routing key rather than by operation, so it kills a successor create that a later message started after the removal — discarding the session it just created and failing the user's message outright.
invalidateRouteOperation(key) deletes routeTokens[key] and invalidates whatever operation creatingSessions.get(key) currently holds. When removeSession has already invalidated and deleted this restore's operation and tombstoned the key, creatingSessions[key] at settle time is the successor create registered by resolve(). The settle therefore invalidates op_new; createAndStoreSession's assertOperationCurrent throws, scheduleDiscardInvalidatedSession destroys the session it just created, and resolve()'s create-branch catch rethrows terminally because neither creatingSessions.has(key) nor toSession.has(key) holds. This is reachable through QQChannel.ts:2122, a restoreSessions() caller that no bridge-recovery readiness gate guards, so /clear plus one following message inside that window is the trigger.
Witness:
probe, same input on both arms (restore parked on loadSession -> removeSession -> resolve() reaches
the create branch -> the parked load settles):
BASE (1feb3804): {"resolveOutcome":{"ok":"session-new"},"newSessionCalls":1,
"discardCalls":["old-alice"],"routedNow":"session-new",
"persisted":{"ch:alice:chat1":{"sessionId":"session-new"}}}
PR (6082d7c2): {"resolveOutcome":{"err":"Session route operation was invalidated"},
"newSessionCalls":1,"discardCalls":["old-alice","session-new"],
"routedNow":undefined,"persisted":{}}
scoped fix : {"resolveOutcome":{"ok":"session-new"},"discardCalls":[],
"routedNow":"session-new"} — all 178 existing SessionRouter tests still pass
| if (this.suspendedDeletionKeys.has(key)) { | |
| this.invalidateRouteOperation(key); | |
| } | |
| if (this.suspendedDeletionKeys.has(key)) { | |
| if (this.creatingSessions.get(key) === operation) { | |
| this.invalidateRouteOperation(key); | |
| } else { | |
| this.invalidateOperation(operation); | |
| } | |
| } |
The fix rests on two premises that were measured, not assumed. A key dropped by another restore's validation loop tombstones without invalidating any operation (SessionRouter.ts:1211-1215), so merely skipping when another owner holds the key would let that restore's settle resurrect a malformed entry — the fix must still invalidate operation itself. And with the scoping applied, the loaded session is no longer reclaimed (discardCalls: [] for old-alice), because creatingSessions.has(key) is then true at the discard guard SessionRouter.ts:1318, whose premise (a successor restore loading the same persisted session id) is false for a fresh create — so a companion change that distinguishes a successor restore from an unrelated create is required, or this trades a failed message for a bridge session leaked at :1318.
Acceptance: add a SessionRouter.test.ts case beside "does not resurrect a route cleared after both reservation passes" that defers loadSession, removes the key, starts router.resolve(...) so a fresh create is in flight, then settles the restore's load, and asserts resolve() resolves to the newly created id and that bridge.discardSession was not called with it. Removing the operation scoping must turn that test red.
中文说明
严重：关闭 R8-4 的落定侧墓碑复查是按「路由键」而非按「操作」失效的，因此会误杀移除之后由后续消息发起的继任创建操作——丢弃它刚创建的会话，并让用户的这条消息直接失败。
invalidateRouteOperation(key) 会删除 routeTokens[key]，并失效 creatingSessions.get(key) 当前持有的任意操作。当 removeSession 已经失效并删除了本次恢复的操作、并为该键写入墓碑后，落定时刻 creatingSessions[key] 中持有的是 resolve() 注册的继任创建操作。于是落定逻辑失效了 op_new：createAndStoreSession 的 assertOperationCurrent 抛错，scheduleDiscardInvalidatedSession 销毁刚创建的会话，而 resolve() 的创建分支 catch 因 creatingSessions.has(key) 与 toSession.has(key) 均为 false 而终止重抛。可达路径：QQChannel.ts:2122 是一个不受 bridge-recovery 就绪门保护的 restoreSessions() 调用方，因此「/clear + 窗口内紧随其后的一条消息」即可触发。
证据（见上）：同一输入在合并基上 resolveOutcome 为 {"ok":"session-new"}、仅 discard old-alice、新会话被持久化；在本 PR 上则为 "Session route operation was invalidated"、old-alice 与 session-new 双双被 discard、routedNow 为 undefined、持久化存储为空。按操作收窄后恢复正常，且现有 178 个 SessionRouter 测试全部通过。
修复依赖两个已实测（而非假设）的前提。其一，被另一次恢复的校验循环丢弃的键会写墓碑但不失效任何操作（SessionRouter.ts:1211-1215），因此「有别人持有该键就跳过」会让那次恢复的落定复活一个非法条目——修复仍必须失效 operation 自身。其二，实测表明按操作收窄后已加载的会话不再被回收（old-alice 的 discardCalls 为空），因为此时 creatingSessions.has(key) 在丢弃守卫 SessionRouter.ts:1318 处为真，而该守卫「继任恢复正在加载同一持久化会话 ID」的前提对全新创建并不成立——所以还需要一个能区分「继任恢复」与「无关创建」的配套修改，否则就是把「消息失败」换成了「在 :1318 处泄漏一个 bridge 会话」。
验收：在 SessionRouter.test.ts 的 "does not resurrect a route cleared after both reservation passes" 旁补一个用例——挂起 loadSession、移除该键、发起 router.resolve(...) 使一个全新创建在途，再让恢复的加载落定，断言 resolve() 解析到新创建的 id 且 bridge.discardSession 未以该 id 被调用。移除按操作收窄后该测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| const leases = | ||
| (reserved.liveRotation?.leases ?? 0) + | ||
| (this.rotationDeltas.get(wipedId)?.leases ?? 0); | ||
| if (reserved.wipedBridgeLive && leases > 0) { |
There was a problem hiding this comment.
[Critical] R11-1: (fix-induced) [certifies-falsely] [regression] The keep-route fallback added to close R11-1 decides from a wipedBridgeLive boolean captured before the await and never re-validated at settle time, so it re-installs a session whose death the restore's own load-window guard has just consumed — re-pointing the route at it, marking it live, and making that false certification durable.
Route K is live with session S and a message has resolved to S but not yet registered its turn, so sessionRoutingLeases[S] is 1. A restore's reservation pass wipes K, capturing wipedSessionId = S, wipedBridgeLive = true and liveRotation.leases = 1, then awaits loadSession(S). S dies mid-load: handleSessionDied(S) → removeSessionId(S) finds nothing in toSession (the wipe already cleared it), so it takes the sessionLoadWindows branch and only marks S in the open load window. The load then resolves and the success path throws 'Restored session died before routing completed' after consuming that mark. That plain Error lands in this branch: leases is still 1 (the captured value is immune to removeSessionId's delete) and wipedBridgeLive is still true, so the router sets toSession[K] = S, adds S back to liveSessionIds and re-applies the captured counters — and the end-of-restore flush writes the dead id to routes.json. The branch's own promised self-heal ("their prompts fail and sessionDied cleans up") cannot fire, because the only sessionDied for S was already consumed. Every later message on K is handed a session the bridge reported dead.
Witness:
PROBE-C, unmodified PR, eager router with a real persist file, one live route holding a routing
lease, restoreSessions() in flight, handleSessionDied('session-1') mid-load, then the load resolves:
"[SessionRouter] Failed to restore session session-1 for key ch:alice:chat1:
Restored session died before routing completed"
"afterDeath": {"toSession":[],"liveSessionIds":[],"leases":[],"tombstones":[]}, "diedReturn":false
"afterRestore": {"toSession":[["ch:alice:chat1","session-1"]],"liveSessionIds":["session-1"],
"leases":[["session-1",1]],
"persisted":{"ch:alice:chat1":{"sessionId":"session-1"}}}
"nextMessageSession":"session-1" "handedBackTheDeadSession":true "discardCalls":[]
branch reverted: the route stays deleted and the next message creates a fresh session (base behaviour)
| if (reserved.wipedBridgeLive && leases > 0) { | |
| if ( | |
| reserved.wipedBridgeLive && | |
| leases > 0 && | |
| !diedDuringLoad && | |
| bridgeUnchangedSinceReservation | |
| ) { |
Re-validate at settle time instead of trusting the pre-await capture; the pattern already exists in this file at loadManagedSession (SessionRouter.ts:875-881), which after its await checks lifecycleGeneration !== this.lifecycleGeneration || bridge !== this.bridge and discards. Capture this.bridge (or lifecycleGeneration) beside wipedBridgeLive in the reservation pass and compare before re-installing, and record where loadWindow.delete(sessionId) fires so the died-during-load case skips the keep-route branch entirely. The sketch above names the two extra conditions; the surrounding body is unchanged.
The fix must not be narrowed to dropping this.liveSessionIds.add(wipedId): private isLive(sessionId) { return this.recoveryMode === 'eager' || this.liveSessionIds.has(sessionId); } (SessionRouter.ts:569-571) ignores that set entirely in the default eager mode, so this.toSession.set(key, wipedId) has to be skipped too. And it must keep the branch's stated purpose for the case it was written for — a load that fails for a reason other than a death report or a bridge swap, with a genuinely held lease, must still keep the route so in-flight messages are not killed mid-turn (SessionRouter.ts:1391-1393).
Acceptance: extend SessionRouter.test.ts "re-routes to the wiped live session when an overlap load fails under held leases" with a death-during-load variant — hold a routing lease on the wiped session, call router.removeSessionId(wipedId) inside the load window so the success path throws 'Restored session died before routing completed', and assert the key is afterwards routed to a fresh session and router.isSessionLive(wipedId) is false. Removing the settle-time revalidation must turn it red.
中文说明
严重：为关闭 R11-1 而新增的「保留路由」回退分支，依据的是在 await 之前捕获、且落定时从不重新校验的 wipedBridgeLive 布尔值，因此会把一个「其死亡事件刚刚被本次恢复的 load-window 守卫消费掉」的会话重新装回路由——重新指向它、标记为存活，并把这份错误的认定写入持久化文件。
路由 K 存活于会话 S，且有一条消息已解析到 S 但尚未登记回合，故 sessionRoutingLeases[S] 为 1。某次恢复的预留阶段抹除 K，捕获 wipedSessionId = S、wipedBridgeLive = true、liveRotation.leases = 1，随后 await loadSession(S)。S 在加载途中死亡：handleSessionDied(S) → removeSessionId(S) 在 toSession 中找不到任何键（抹除已清空它），于是走 sessionLoadWindows 分支，仅把 S 标记进打开的加载窗口。随后加载返回，成功路径在消费该标记后抛出 'Restored session died before routing completed'。这个普通 Error 落入本分支：leases 仍为 1（捕获值不受 removeSessionId 删除的影响），wipedBridgeLive 仍为 true，于是路由器执行 toSession[K] = S、把 S 重新加入 liveSessionIds 并重放捕获的计数器——恢复结束时的 flush 把这个已死亡的 id 写入 routes.json。分支自己承诺的自愈（「它们的 prompt 会失败，sessionDied 会清理」）无法发生，因为 S 唯一的一次 sessionDied 已被消费。此后 K 上的每条消息都会被交给一个 bridge 已报告死亡的会话。
证据（见上 PROBE-C）：死亡发生后 toSession/liveSessionIds/leases/tombstones 全空、diedReturn 为 false；恢复结束后 toSession 重新持有 ch:alice:chat1 → session-1、liveSessionIds 含 session-1、租约恢复为 1，且持久化文件写入 sessionId: "session-1"；下一条消息拿到的仍是 session-1（handedBackTheDeadSession: true），discardCalls 为空。回退该分支后，路由保持删除、下一条消息创建全新会话（即合并基行为）。
修复不应收窄为「只去掉 this.liveSessionIds.add(wipedId)」：private isLive(sessionId) { return this.recoveryMode === 'eager' || this.liveSessionIds.has(sessionId); }（SessionRouter.ts:569-571）在默认 eager 模式下完全忽略该集合，因此 this.toSession.set(key, wipedId) 也必须跳过。同时必须保留该分支为其目标场景保留的语义——当加载失败的原因不是死亡报告或 bridge 更换、且确实持有租约时，仍要保留路由，以免在途消息被中途杀掉（SessionRouter.ts:1391-1393）。建议的落定时刻重新校验模式在本文件已有先例：loadManagedSession（SessionRouter.ts:875-881）在 await 之后检查 lifecycleGeneration !== this.lifecycleGeneration || bridge !== this.bridge 并 discard。
验收：为 SessionRouter.test.ts 的 "re-routes to the wiped live session when an overlap load fails under held leases" 补一个「加载期间死亡」变体——在待抹除会话上持有路由租约，在加载窗口内调用 router.removeSessionId(wipedId) 使成功路径抛出 'Restored session died before routing completed'，断言此后该键路由到一个全新会话且 router.isSessionLive(wipedId) 为 false。移除落定时刻的重新校验后该测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| if ( | ||
| !this.creatingSessions.has(key) && | ||
| !this.wipedRouteState.has(key) | ||
| ) { |
There was a problem hiding this comment.
[Critical] R12-1: [certifies-falsely] [new-surface] The connectivity-abort cleanup pass tests only whether an operation or a wipe capture exists for the key, never whether it is still this restore's, and that guard sits above the reservation settle. Both directions are wrong: when it passes, it re-adds a route that was deliberately removed mid-window and the end-of-restore flush writes it to disk, undoing a /clear durably; when it skips, it also skips reserved.reservation.reject(...), so a parked resolve() waiter never settles and the user's message is silently dropped.
Resurrection. Restore A reserves keys from a snapshot holding K → S with persistence suspended. /clear for K lands (removeSession invalidates A's operation, deleteByKey(K), tombstoneSuspendedKey(K)). A's load for another key throws BridgeConnectivityError, so aborted = true and the loop breaks. The abort pass reaches K: creatingSessions.has(K) is true because the user's next message started a fresh create there, so the guard passes and readdAbortedRoute — which never consults suspendedDeletionKeys — sees !toSession.has(K) and writes toSession[K] = S. The flush then runs (the tombstone's persist() set persistRequestedWhileSuspended), clears the tombstones and writes K → S to disk. The next message resolves to S and, in eager mode, isLive() is unconditionally true, so the router serves a session that was cleared and never loaded on the current bridge. This needs no follow-up message at all via removeSessionId, which tombstones without calling deleteByKey and so leaves wipedRouteState[K] set.
Hang. The same guard, the other way. A message arrives mid-restore and resolve() parks on the reservation for K — the designed behaviour ("Reserve every persisted key up front so inbound messages during restart wait for restore"). K's owner then sends /clear, which is dispatched before routing and so is not itself parked: removeSession → invalidateRouteOperation(K) (deletes the creatingSessions entry but does not settle the promise) → deleteByKey(K), whose first line clears wipedRouteState[K] and then early-returns null. The restore aborts on connectivity; the cleanup loop evaluates K, finds neither creatingSessions.has(K) nor wipedRouteState.has(K), and continues past reserved.reservation.reject(...). restoreSessions() returns and drops its local reservations map, so nothing can ever settle that promise. The parked resolve() — and the handleInbound awaiting it, which has no timeout — never returns: the message is dropped with no reply and no error, and the promise plus its captured envelope leaks for the process lifetime. A later restore cannot rescue it, because the operation is no longer in creatingSessions.
Witness:
resurrection arm — A parks on loadSession('sess-alice'); /clear for bob's route lands inside the
window; bob's next message starts a fresh create; A's load then rejects with BridgeConnectivityError:
INTACT: {"bobRouteNow":"sess-bob",
"allRoutes":[["ch:alice:chat1","sess-alice"],["ch:bob:chat2","sess-bob"]],
"persistedOnDisk":{"ch:alice:chat1":{...},"ch:bob:chat2":{"sessionId":"sess-bob"}}}
FIXED (identity guard + tombstone check):
{"bobRouteNow":undefined,"allRoutes":[["ch:alice:chat1","sess-alice"]],
"persistedOnDisk":{"ch:alice:chat1":{...}}} ← alice's un-attempted route still kept
hang arm — P1 and both control arms:
ARM /clear + connectivity abort : waiterState=pending ← never settles (after await + 30 microtask
drains + a 50 ms timer)
ARM abort, no /clear : waiterState=resolved ← control
ARM /clear, no abort : waiterState=rejected ← control
FIX (reject moved above guard) : waiterState=rejected, /clear-no-abort arm unchanged;
whole package 1366 tests pass
Settle unconditionally and keep only the re-add gated — a promise already settled ignores a second settle, so this is a no-op for restored, failed-load-pruned and already-invalidated keys. Move reserved.reservation.reject(new BridgeConnectivityError('Session restore aborted: bridge disconnected')) above the guard, then make the re-add ownership- and tombstone-aware: skip keys removed mid-window (if (this.suspendedDeletionKeys.has(key)) continue; after the reject) and test identity rather than existence (this.creatingSessions.get(key) !== reserved.operation → continue), so a successor-owned key is left to its owner.
The reject must move above the guard; the guard must not be deleted. readdAbortedRoute exists to keep un-attempted routes for retry — SessionRouter.ts:1370-1373: "Keep this and every un-attempted route (re-added after the loop) so crash recovery's restore can retry them; a pruning flush would permanently lose routes the restore never reached" — and the pinned test "keeps un-attempted routes when the bridge dies mid-restore" asserts expect(router.getAll()).toHaveLength(3), that the persist file keeps all 3 keys, and that a retry restores 3. A key this restore reserved but never reached still holds this restore's own operation, so the identity guard keeps re-adding exactly those.
Acceptance: two SessionRouter.test.ts cases. (a) Beside "keeps un-attempted routes when the bridge dies mid-restore": write a 3-key snapshot, defer loadSession, call router.removeSession('ch','alice','chat1') after the reservation pass, then reject the load with BridgeConnectivityError; assert router.getSession('ch','alice','chat1') is undefined and persisted['ch:alice:chat1'] is undefined while getAll() still holds the other two. (b) Waiter settlement on the abort path, which no current abort assertion covers: persist two routes, make loadSession reject with BridgeConnectivityError on the first key, park const waiter = router.resolve('ch','alice','chat1') on the second, call removeSession for that key mid-restore, await the restore, and assert the waiter settled rather than hanging. Moving the reject back below the guard must turn (b) red; dropping the tombstone/identity guard must turn (a) red.
中文说明
严重：连通性中止后的清理循环只检查该键「是否存在某个操作或某份抹除捕获」，从不检查它是否仍属于本次恢复；而这个守卫位于预留 settle 之上。两个方向都错：守卫通过时，它会把窗口中被刻意移除的路由重新加回，并由恢复结束的 flush 写入磁盘，从而持久化地撤销一次 /clear；守卫跳过时，它同时跳过了 reserved.reservation.reject(...)，于是停在预留上的 resolve() 等待者永不落定，用户的消息被静默丢弃。
复活。 恢复 A 在持久化挂起状态下，从快照中预留了含 K → S 的若干键。K 的 /clear 落下（removeSession 失效 A 的操作、deleteByKey(K)、tombstoneSuspendedKey(K)）。A 在另一个键上的加载抛出 BridgeConnectivityError，于是 aborted = true 并 break。中止清理走到 K：因为用户的下一条消息已在该键上发起全新创建，creatingSessions.has(K) 为真，守卫通过；而 readdAbortedRoute 从不查询 suspendedDeletionKeys，它看到 !toSession.has(K) 便写入 toSession[K] = S。随后 flush 执行（墓碑的 persist() 已置位 persistRequestedWhileSuspended），清空墓碑并把 K → S 写入磁盘。下一条消息解析到 S，且在 eager 模式下 isLive() 恒为真，于是路由器交出一个已被清除、且从未在当前 bridge 上加载的会话。经 removeSessionId 触发时甚至不需要后续消息——它写墓碑但不调用 deleteByKey，因此 wipedRouteState[K] 仍在。
挂起。 同一个守卫的反方向。一条消息在恢复途中到达，resolve() 停在 K 的预留上——这正是设计行为（「预先预留每个持久化键，使重启期间的入站消息等待恢复」）。随后 K 的属主发来 /clear，它在路由之前分派，因此自身不会被挂起：removeSession → invalidateRouteOperation(K)（删除 creatingSessions 条目，但不落定该 promise）→ deleteByKey(K)，其首行清掉 wipedRouteState[K] 后提前返回 null。恢复因连通性中止；清理循环评估 K，发现 creatingSessions.has(K) 与 wipedRouteState.has(K) 均为假，于是 continue 跳过了 reserved.reservation.reject(...)。restoreSessions() 返回并丢弃其局部 reservations map，因此再无任何东西能落定那个 promise。停在上面的 resolve()——以及 await 它、且没有超时的 handleInbound——永不返回：消息被丢弃，既无回复也无错误，promise 连同其捕获的 envelope 在进程生命周期内泄漏。后续恢复也救不回来，因为该操作已不在 creatingSessions 中。
证据（见上）：复活臂中，未修复时 bobRouteNow 为 sess-bob 且 ch:bob:chat2 被写入磁盘；加入身份守卫与墓碑检查后 bobRouteNow 为 undefined，同时 alice 未尝试的路由仍被保留以供重试。挂起臂中，「/clear + 连通性中止」组合下 waiterState=pending（await 之后再加 30 次微任务排空与一个 50 ms 定时器仍不落定），而两个对照臂分别 resolved 与 rejected；把 reject 移到守卫之上后失败臂翻转为 rejected、对照臂不变，整个 package 1366 个测试通过。
修复：无条件落定，只对「重新加回」设门。已落定的 promise 会忽略第二次落定，因此对已恢复、因加载失败被剪除、以及已失效的键都是空操作。把 reserved.reservation.reject(new BridgeConnectivityError('Session restore aborted: bridge disconnected')) 移到守卫之上；随后让重新加回同时具备归属感知与墓碑感知：跳过窗口中被移除的键（reject 之后 if (this.suspendedDeletionKeys.has(key)) continue;），并以身份而非存在性判断（this.creatingSessions.get(key) !== reserved.operation → continue），使由继任者持有的键交给其属主处理。
reject 必须移到守卫之上，守卫本身不能删除。readdAbortedRoute 的存在意义是保留未尝试的路由以供重试——SessionRouter.ts:1370-1373：「保留本条以及每一条未尝试的路由（循环后重新加回），以便 crash recovery 的恢复能重试它们；一次剪枝式 flush 会永久丢失恢复从未触及的路由」——且既有测试 "keeps un-attempted routes when the bridge dies mid-restore" 断言 expect(router.getAll()).toHaveLength(3)、持久化文件保留全部 3 个键、且重试能恢复 3 条。本次恢复预留但从未触及的键仍持有本次恢复自己的操作，因此身份守卫恰好会继续加回这些键。
验收：两个 SessionRouter.test.ts 用例。(a) 在 "keeps un-attempted routes when the bridge dies mid-restore" 旁：写入 3 键快照、挂起 loadSession、在预留阶段之后调用 router.removeSession('ch','alice','chat1')，再以 BridgeConnectivityError 拒绝加载；断言 router.getSession('ch','alice','chat1') 为 undefined 且 persisted['ch:alice:chat1'] 为 undefined，同时 getAll() 仍持有另外两条。(b) 中止路径上的等待者落定（现有中止断言均未覆盖）：持久化两条路由，使 loadSession 在第一个键上以 BridgeConnectivityError 拒绝，在第二个键上停住 const waiter = router.resolve('ch','alice','chat1')，恢复途中对该键调用 removeSession，await 恢复完成，断言 waiter 已落定而非挂起。把 reject 移回守卫之下必须使 (b) 变红；去掉墓碑/身份守卫必须使 (a) 变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| this.toSession.delete(key); | ||
| this.tombstoneSuspendedKey(key); | ||
| removed = true; |
There was a problem hiding this comment.
[Critical] R12-2: [certifies-falsely] [regression] A removal by session id can never tombstone the key when a restore has already wiped it — this loop iterates toSession, which the reservation pass emptied — so the restore's settle has no signal that the session was deliberately killed, and its keep-in-flight branch resurrects and persists a session whose death was already delivered and consumed.
A restore is suspended; route K → S is live and holds a routing lease (the !cmd shell and /btw paths hold resolve()'s lease across real awaits). The reservation pass wipes K, captures liveRotation.leases = 1 and wipedBridgeLive = true, and seeds rotationDeltas[S]. S then dies: onSessionDied → handleSessionDied(S) → removeSessionId(S) finds no key in toSession, so it calls neither invalidateRouteOperation(K) nor tombstoneSuspendedKey(K); it only re-purges the already-empty per-session maps and marks S in the load window, which is what makes the pending loadSession(S) fail. The settle lands in the failure branch with leases = 1 and wipedBridgeLive true, so it re-adds toSession[K] = S, liveSessionIds.add(S) and sessionRoutingLeases[S] = 1, and the flush persists the dead id. Because liveSessionIds again contains S, the next message takes the eager fast path and is handed a session the bridge already reported dead.
The branch's comment promises "their prompts fail and sessionDied cleans up", but neither shipped bridge can deliver that second sessionDied, so nothing re-runs the cleanup and the route stays durably wrong.
Witness:
PROBE-C, unmodified PR — measuring this finding's own anchor:
"afterDeath": {"toSession":[],"liveSessionIds":[],"leases":[],"tombstones":[]}, "diedReturn":false
"afterRestore": {"toSession":[["ch:alice:chat1","session-1"]],
"persisted":{"ch:alice:chat1":{"sessionId":"session-1"}}},
"handedBackTheDeadSession":true
self-heal premise checked on both shipped bridges:
AcpBridge NEVER emits sessionDied — its only occurrence is the comment at AcpBridge.ts:157
("Do not emit sessionDied here: a full ACP process exit is handled by channel start crash
recovery"); prompt wraps conn.prompt in settleOnChildExit and simply rejects, btw throws
"Unknown ACP session".
DaemonChannelBridge.dropSession does `const session = this.removeSessionBinding(sessionId);
if (!session) return;` BEFORE `this.emit('sessionDied', ...)`, and removeSessionBinding returns
undefined once sessions.delete has run — so a second death for the same session cannot re-emit.
Make the wipe capture cancellable by session id, not only by routing key: in removeSessionId (and the lazy half of handleSessionDied), when no key maps to sessionId, scan wipedRouteState for entries whose wipedSessionId === sessionId and drop the capture (wipedRouteState.delete(key) plus rotationDeltas.delete(sessionId)) — or record the id in a suspendedDeletionIds set that the failure branch consults before re-adding wipedId, mirroring how suspendedDeletionKeys already gates the reservation and settle paths.
The keep-in-flight branch is deliberate for a session that merely failed to load but is still alive — SessionRouter.ts:1391-1393: "Messages routed to the wiped session before this restore took the key are still in flight on it: keep the route so they are not killed mid-turn." The fix must therefore gate on an observed death or removal of that session id, not on the load failure itself, or it drops routes whose in-flight turns are genuinely still running.
Acceptance: extend SessionRouter.test.ts "re-routes to the wiped live session when an overlap load fails under held leases" — after the second restoreSessions() has reserved the key and before its mocked loadSession rejects, call router.handleSessionDied('old-alice'); assert router.getSession('ch','alice','chat1') is undefined, isSessionLive('old-alice') is false, and the flushed store has no ch:alice:chat1 key. Removing the by-id cancellation must turn it red.
中文说明
严重：当一次恢复已经抹除某键后，按会话 id 执行的移除永远无法为该键写入墓碑——这个循环遍历的是 toSession，而预留阶段已把它清空——于是恢复的落定得不到「该会话是被刻意杀掉」的信号，其「保留在途」分支会复活并持久化一个死亡事件早已投递并被消费掉的会话。
恢复处于挂起状态；路由 K → S 存活并持有路由租约（!cmd shell 与 /btw 路径会跨真实 await 持有 resolve() 的租约）。预留阶段抹除 K，捕获 liveRotation.leases = 1 与 wipedBridgeLive = true，并种下 rotationDeltas[S]。随后 S 死亡：onSessionDied → handleSessionDied(S) → removeSessionId(S) 在 toSession 中找不到任何键，因此既不调用 invalidateRouteOperation(K) 也不调用 tombstoneSuspendedKey(K)；它只是重复清理那些已为空的按会话 map，并把 S 标记进加载窗口——而这正是使待决的 loadSession(S) 失败的原因。落定进入失败分支时 leases = 1、wipedBridgeLive 为真，于是重新加回 toSession[K] = S、liveSessionIds.add(S) 与 sessionRoutingLeases[S] = 1，flush 把这个已死亡的 id 持久化。由于 liveSessionIds 重新包含 S，下一条消息走 eager 快路径，被交给一个 bridge 已报告死亡的会话。
该分支的注释承诺「它们的 prompt 会失败，sessionDied 会清理」，但两个已交付的 bridge 都无法投递这第二次 sessionDied，因此没有任何东西会重新执行清理，路由会持久地保持错误状态。
证据（见上 PROBE-C）：死亡发生后 tombstones 为空、diedReturn 为 false；恢复结束后 toSession 重新持有 ch:alice:chat1 → session-1、持久化文件写入该 id、handedBackTheDeadSession 为 true。自愈前提在两个 bridge 上均被否证：AcpBridge 从不发出 sessionDied（文件中唯一出现处是 AcpBridge.ts:157 的注释），prompt 只是拒绝、btw 抛出 "Unknown ACP session"；DaemonChannelBridge.dropSession 在 this.emit('sessionDied', ...) 之前执行 const session = this.removeSessionBinding(sessionId); if (!session) return;，而 removeSessionBinding 在 sessions.delete 之后返回 undefined，因此同一会话的第二次死亡无法再次发出事件。
修复：让抹除捕获可以按会话 id 取消，而不只按路由键。在 removeSessionId（以及 handleSessionDied 的 lazy 半边）中，当没有任何键映射到 sessionId 时，扫描 wipedRouteState 中 wipedSessionId === sessionId 的条目并丢弃该捕获（wipedRouteState.delete(key) 加 rotationDeltas.delete(sessionId)）；或者把该 id 记入一个 suspendedDeletionIds 集合，由失败分支在重新加回 wipedId 之前查询——与 suspendedDeletionKeys 已经守卫预留与落定两条路径的方式对称。
「保留在途」分支对于「只是加载失败但依然存活」的会话是刻意设计——SessionRouter.ts:1391-1393：「在本次恢复接管该键之前已路由到被抹除会话的消息仍在其上在途：保留路由，以免它们被中途杀掉」。因此修复必须以「观察到该会话 id 的死亡或移除」为门，而不是以加载失败本身为门，否则会丢弃那些在途回合确实仍在运行的路由。
验收：扩展 SessionRouter.test.ts 的 "re-routes to the wiped live session when an overlap load fails under held leases"——在第二个 restoreSessions() 已预留该键、其被 mock 的 loadSession 拒绝之前，调用 router.handleSessionDied('old-alice')；断言 router.getSession('ch','alice','chat1') 为 undefined、isSessionLive('old-alice') 为 false，且 flush 后的存储中不存在 ch:alice:chat1 键。移除按 id 取消后该测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
|
🐑 Merge conflict with main detected — dispatched the autofix loop to resolve it. / 检测到与 main 的合并冲突，已触发 autofix 处理。 |
|
🤖 Could not produce a passing fix for this feedback (round 1/10). This item now needs a human; the loop stays engaged and still picks up new feedback and base conflicts, but will not retry this item on its own. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. 中文说明🤖 未能为该反馈产生可通过验证的修复（第 1/10 轮）。此项现在需要人工处理；循环保持在线，仍会拾取新反馈与 base 冲突，但不会自行重试此项。 Run log: https://github.com/QwenLM/qwen-code/actions/runs/34036615107 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Could not produce a passing fix for this feedback (round 2/10). This item now needs a human; the loop stays engaged and still picks up new feedback and base conflicts, but will not retry this item on its own. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. 中文说明🤖 未能为该反馈产生可通过验证的修复（第 2/10 轮）。此项现在需要人工处理；循环保持在线，仍会拾取新反馈与 base 冲突，但不会自行重试此项。 Run log: https://github.com/QwenLM/qwen-code/actions/runs/34039207194 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Addressed the latest review feedback (round 3/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/10 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #8927Feedback triageThis round's feedback contained no review comments (no reviews, no inline
What was done1. Merged
|
5 participants
What this PR does
Adds a per-channel
sessionRotationoption that bounds how long a route keeps the same session. When the current session on a route is past its bound, the next message on that route starts a fresh session instead of reusing it. Two bounds are supported —maxTurns(messages routed to the session) andmaxAgeHours(wall-clock age) — and either may be set on its own; whichever is hit first rotates.The bound is checked before a message reuses a session, so it caps what the session carries into a turn rather than what it is left holding after one. It is checked on both the live-reuse and the lazy-reload path, so a route cannot dodge its bound by having been evicted from memory, and it is skipped while a session creation is already in flight on that key — invalidating that operation would fail the concurrent message instead of rotating it, and the next message enforces the bound just as well.
Turn counts and start times persist alongside the routes, so a daemon restart cannot reset a bound, and they carry across a session ID change when a reload returns a new ID. Channels with no bound configured skip the bookkeeping entirely: no counters are tracked, the on-disk route shape is unchanged, and there is no extra persist per message. Non-positive or non-finite bounds are rejected at config-parse time, and defensively normalized in the router so a hand-edited store cannot make a channel rotate on every single message.
Omitting
sessionRotationpreserves today's behavior exactly.Why it's needed
SessionRoutermaps a routing key to a session ID and reuses that session for every later message on the key, with nothing bounding how large it gets. A long-lived route grows monotonically until it passes the model's context window; from that point on every message on that route fails while the rest of the channel keeps working, and recovery means finding the wedged route and clearing it by hand (/clearin the chat, or removing the route from the daemon'sroutes.json).I hit this on a DingTalk Q&A bot with
sessionScope: "thread". One group thread had been accumulating since July 27 — 8577 entries / 21 MB in the session JSONL, peakpromptTokenCount849,748. At ~327k prompt tokens every turn began failing upstream, retrying 7 times per turn before giving up, while other threads on the same channel, model, and credentials answered normally. Replaying the failing thread's own tail through the provider API succeeded, confirming the failure was specific to that accumulated session rather than the channel or the model. The operator-visible symptom is "the bot is down" when one route is wedged.Auto-compaction does not cover this: it is driven by the client's configured context window, so when that is larger than what the endpoint actually serves for the session, the wall arrives before compaction ever triggers.
sessionScopealready decides how routes are partitioned; there was no knob for how long a partition lives. Chat channels are the case that needs one — a group thread has no natural end, unlike a CLI session a user closes.Reviewer Test Plan
How to verify
Unit tests cover the behavior end to end. From the repo root:
The added
session rotationblock asserts: no rotation when unconfigured; a new session oncemaxTurnsis reached; only the route that hit the bound rotates while a sibling route keeps its session; a channel without a bound is unaffected when another channel has one;maxAgeHoursrotates on elapsed time (fake timers); non-positive bounds are ignored rather than rotating every message; turn counts survive a restore so a restart cannot reset the bound; and a route store written before this change restores cleanly and starts its clock at the next message instead of rotating on sight.Config parsing tests assert the bounds round-trip, stay
undefinedwhen omitted, and that a non-positive bound is rejected with a clear message.To confirm manually, configure a channel with
"sessionRotation": { "maxTurns": 2 }, send three messages, and observe the third get a new session ID — the router logs[SessionRouter] Rotated session for <channel>: <id> reached its configured limit; starting a new session.and the bot no longer recalls the first two messages.Evidence (Before & After)
N/A — no TUI surface. Behavior change is in routing and is covered by the unit tests above.
Full suites run locally:
Tested on
Environment (optional)
Unit tests only, via
npx vitest runper package on Linux / Node 22.Risk & Scope
sessionRotationchanges nothing. When a bound is configured, each message costs one extra smallroutes.jsonwrite to persist the turn counter; channels without a bound are exempt from that write./clear, so participants get no other signal). WithsessionScope: single, only the triggering chat is notified; other chats sharing the session see the reset without a notice.sessionRotationcannot be combined withmultiSession— named tasks resolve sessions without consulting the rotation gate, so the bound would be accepted but never fire. The combination is rejected at config parse and in the managed settings store.get_context_usageexists only on the SDK control path), so a token bound would need a new bridge capability. Turn count and age are coarser but keep a route from growing without limit, and a token bound can be added later behind the same config key.feat, not arefactor. It touchespackages/channels/base(router, types, one wiring line inChannelBase) andpackages/cli/src/commands/channel(config parsing plus one wiring line in each ofstart.tsanddaemon-worker.ts). The new router method has exactly three call sites, all listed above.Linked Issues
Closes #8926
中文说明
这个 PR 做了什么
为频道新增
sessionRotation配置，用于限制一个路由复用同一会话的时长。当路由上的当前会话超出配置的限度时，下一条消息会开一个全新会话，而不是继续复用。支持两个限度——maxTurns（路由到该会话的消息数）和maxAgeHours（自然时间年龄），二者可单独设置，先达到的那个触发轮换。限度在消息复用会话之前检查，因此它约束的是会话带入本轮的上下文量，而不是本轮结束后残留的量。检查同时覆盖存活复用和惰性重载两条路径，避免路由因为被逐出内存而绕过限度；如果该 key 上已有创建操作在途则跳过本次检查——此时作废该操作会让并发的那条消息失败而不是完成轮换，而下一条消息同样能落实限度。
轮次计数和起始时间与路由一起持久化，因此守护进程重启不会重置限度；当重载返回新的会话 ID 时，这些计数会随之迁移。未配置限度的频道完全跳过这套记账：不跟踪计数器，磁盘上的路由结构不变，也没有每条消息的额外写盘。非正数和非有限值在配置解析阶段就会报错，路由层还会再做一次防御性归一化，避免手工改坏的存储导致频道对每条消息都轮换。
不填
sessionRotation时行为与当前完全一致。为什么需要
SessionRouter把路由键映射到会话 ID，之后该键上的每条消息都复用这个会话，没有任何机制限制它增长到多大。长期存在的路由会单调增长，直到超过模型上下文窗口；从那一刻起该路由上的每条消息都会失败，而频道其余部分一切正常，恢复手段是找到卡死的路由并手工清理（聊天里/clear，或从守护进程的routes.json中移除该路由）。我在一个
sessionScope: "thread"的钉钉答疑机器人上遇到了这个问题。某个群 thread 从 7 月 27 日起持续累积——会话 JSONL 已有 8577 条 / 21 MB，promptTokenCount峰值 849748。在约 32.7 万 prompt token 时，每一轮都开始在上游失败，每轮重试 7 次后放弃，而同一频道、同一模型、同一凭证下的其他 thread 回答完全正常。把失败 thread 自己的上下文尾部通过 provider API 回放是成功的，这确认了故障绑定在那个累积起来的会话上，而非频道或模型。运维视角看到的现象是「机器人挂了」，实际只是一个路由卡死。自动压缩覆盖不了这种情况：它由客户端配置的上下文窗口驱动，当该配置大于端上实际为会话提供的窗口时，硬墙会在压缩触发之前就到来。
sessionScope已经决定了路由如何划分，但没有任何开关决定一个划分能活多久。聊天频道正是需要这个开关的场景——群 thread 没有自然终点，不像用户会主动关闭的 CLI 会话。审阅者验证方案
如何验证
单元测试完整覆盖了该行为。在仓库根目录执行：
新增的
session rotation测试块断言了：未配置时不轮换；达到maxTurns后开新会话；只有触达限度的那个路由轮换、同级路由保持原会话；某个频道配置了限度时其他频道不受影响；maxAgeHours按流逝时间触发轮换（使用 fake timers）；非正数限度被忽略而不是每条消息都轮换；轮次计数在恢复后仍然有效，重启无法重置限度；本次改动之前写入的路由存储能正常恢复，并从下一条消息开始计时而不是立刻轮换。配置解析测试断言了限度能正确往返、省略时保持
undefined、以及非正数限度会带清晰信息报错。手工确认方式：给某个频道配置
"sessionRotation": { "maxTurns": 2 }，发三条消息，观察第三条拿到新的会话 ID——路由层会输出[SessionRouter] Rotated session for <channel>: <id> reached its configured limit; starting a new session.，且机器人不再记得前两条消息。证据（前后对比）
N/A——没有 TUI 界面改动。行为变更在路由层，由上述单元测试覆盖。
本地跑过的完整套件：
测试平台
运行环境（可选）
仅单元测试，在 Linux / Node 22 上按包执行
npx vitest run。风险与范围
sessionRotation则什么都不变。配置了限度后，每条消息会多一次很小的routes.json写入以持久化轮次计数；未配置限度的频道不承担这次写入。/clear不同，参与者没有其他途径感知）。sessionScope: single时只有触发聊天收到提示，共享该会话的其他聊天只会看到静默重置。sessionRotation不能与multiSession组合——命名任务解析会话不经过轮换门控所在的SessionRouter.resolve，限度会被接受但永远不会触发。该组合在配置解析和托管设置存储两处都会被拒绝。get_context_usage只存在于 SDK 控制通路），因此 token 限度需要新增 bridge 能力。轮次和年龄更粗糙，但足以防止路由无限增长，后续可以在同一个配置键下补充 token 限度。feat而非refactor。改动涉及packages/channels/base（路由器、类型、ChannelBase中一行接线）和packages/cli/src/commands/channel（配置解析，以及start.ts和daemon-worker.ts各一行接线）。新增的路由器方法恰好有三个调用点，均已在上文列出。关联 Issue
Closes #8926